feat: support for multiple entry points / multi-project support#199
Conversation
|
Question I am not sure if I added correctly the new test-project. Please double-check here :) |
|
Thanks Michael, I will start the review ASAP (but could take a while) |
michaelbe812
changed the title
Looking forward to:) |
|
Sorry, 2 weeks have passed, and I still haven't found the time for the review. What came to mind, though, were some suggestions for the API.
|
|
No worries Rainer, just take you time. I might also have some longer response times in the next weeks because of other priorities and duties :-) Inline your suggestions, makes absolutely sense. To summit up: And add another option I guess it only makes sense to have either Maybe we can also come up with a more self-explanatory/ streamlined API for the different workspace types (I have no idea yet). What's the idea behind the |
|
Just as info I already incorporated your feedback regardign I did also add a few test cases for multi-project setups |
|
@michaelbe812 thanks for letting me know. I will have more time in the upcoming two weeks. Maybe I manage to do the review over the weekend, but no gurantee |
Don't feel pressured 😇 by accident I just had a bit of time. Looking forward to your feedback:) |
rainerhahnekamp
left a comment
rainerhahnekamp
left a comment
There was a problem hiding this comment.
Hey @michaelbe812,
Thanks again for this one — it's going to be an important feature and will
really improve the combination of Nx and Sheriff.
I left a few comments regarding implementation details, but I would also like
to discuss how we handle entryPoints internally.
Currently, a large part of Sheriff processes only a single entryPoint,
meaning we don't need to check whether other entryPoints exist.
My idea would be that entryPoints should not be used after the configuration
has been parsed. During parsing, we should determine whether the entryFile
comes directly from entryFile or is derived from entryPoints.
To support this, we would also need to adjust the Configuration.
I suggest removing entryFile and introducing something like finalEntryFile
(or a better name if you have one).
The renaming should make it clear that this is not simply the entryFile from
the configuration, but a resolved value — either specified directly or derived
from entryPoints. A small comment in the code would likely be helpful to make
this clear.
parseConfig would be responsible for resolving this field.
As a result, all the CLI changes you introduced would not be necessary anymore
— the only change would be to use finalEntryFile (or whatever name we choose)
instead of entryFile.
I would still keep entryPoints in the Configuration, because I imagine
that in the future, CLI commands like verify, export, and list could offer
an option to run through all entryPoints.
However, I would treat that as a separate PR.
What do you think about this approach?
Regarding the review:
- I checked only the core changes for now and haven't looked into
angular-v-multiyet. - I expect there will be more review rounds, as this is a complex topic — but
I'll try to respond quicker next time!
Summary
- Decide on the
finalEntryFile(or alternative name) and implement it. - Update documentation accordingly.
|
To keep you updated Rainer: |
|
@rainerhahnekamp |
|
Thanks
Sorry, but I don't see any question from you, if I click on that link. |
It is regarding this comment from you:
I did ask the following: Which behavior should we support? When we have entryPoints set and a user is calling npx sheriff verify it will run verify for each project right now. I see the following possibilities:
Which option do you prefer? Any alternative option? |
|
@ michaelbe812 When I wrote the initial review, I hadn't considered the option of running all entry points by default. But after you showed it to me, I had some time to think it through — and I now believe that running all when no As for the Let me know what you think. |
I would probably keep the behavior as it is right now, like you described. this would also mean for me that we do not need the If wished we could for instance also add in the future a CLI argument |
|
@rainerhahnekamp Kindly test thorougly on your own the new functionality. Actually I do not know why the Pipeline is failing. For me it looks like the |
|
@michaelbe812, just to be sure. we are here on the same page. no // will fail and demand a file as parameter
npx sheriff verify // ⛔️
// will work and use entryFile
npx sheriff verify // ✅
// will run src/main.ts - even if entryFile is different
npx sheriff verify src/main.ts // ✅no // will run on entryPoints
npx sheriff verify // ✅
// run entryPoint with that that name -> if holidays exist in entryPoints
npx sheriff verify holidays // ✅
// fallback to file if entryPoint for apps/hotels/src/main.ts does not exist - I think that this is a new requirement
npx sheriff verify apps/hotels/src/main.ts // ✅Please confirm or correct me. At the moment, I don't see a need for |
Regarding the need of Regaring the examples you provided: |
|
@rainerhahnekamp It all works as expected. The only option which is not available atm is However I did already see this requirement and opened an issue yesterday I would like to do this after this PR is merged if it is okay for you? However I do not see why the build pipeline is failing, could you assist here? I think we can soon get this PR over the line 🚀 |
rainerhahnekamp
left a comment
rainerhahnekamp
left a comment
There was a problem hiding this comment.
Hello @michaelbe812,
So first of all thanks a lot again.
I finally found the time to do a thorough review. It might look like a lot of comments, but I don’t think they should take too much time to address. For a few points, I’d also be interested in your thoughts.
You mentioned that you plan to implement npx sheriff verify [entryPointName] in a separate PR. I did the review assuming that this PR already includes support for that. Is there a particular reason why this feature should be handled separately?
I'm not sure why the integration tests are failing. From what I can see, verify exits with an error in the angular-i test project — which it shouldn't.
Lastly, I think we should also add a bit of documentation. You’ll find the docs in the /docs directory. I think we can create an own page for that.
| 'entryFile', | ||
| 'isConfigFileMissing', | ||
| 'barrelFileName', | ||
| 'entryPoints', |
There was a problem hiding this comment.
Please also add tests where you check that either entryPoints or entryFile exists. And another one if entryPoints exists, that it is not empty.
|
|
||
| const data = getProjectData(entryFile, fs.cwd(), { includeExternalLibraries: true }); | ||
| cli.log(JSON.stringify(data, null, ' ')); | ||
| for (const entry of projectEntries) { |
There was a problem hiding this comment.
I think we need to adopt that one as well.
How do we want to do it? There are some tools which use this function to load data. So we have to be careful.
I see two options:
- Every file gets new property called project with the project's name. In that sense, we would just have an additional property which will hopefully not break anything.
- We create a nested json, where all the entries get the project's name as their parent property.
What is your opinion on that?
There was a problem hiding this comment.
I guess option 1 will be the more "friendly" option as it will/should not break anything as you already pointed out.
So I would go with this approach.
Option 2 we could keep in mind for a future major version and/or as alternative format for exportData
There was a problem hiding this comment.
@rainerhahnekamp
As far as I checked everything correctly we will need to pass either Entry to getProjectData instead of the entryPath or add another argument projectName.
Nevertheless this would introduce a breaking change because getProjectData is exported from the main index.ts.
How should be handle this?
There was a problem hiding this comment.
I am not sure if getProjectData should be aware of entryPoints vs entryFile. In the end this is really an API and not a CLI command.
There was a problem hiding this comment.
Then I don't know what to do here.
For introducing the new property projectName we must be aware of the entryPoints because the key is the projectName, right?
Also what to do in case of entryFile - there we don't actually have a projectName from my understanding.
There was a problem hiding this comment.
Yes, you are right. Sorry for the confusion.
We need to have the possibility to pass on the projectName as well (if there are more projects involved).
michaelbe812
force-pushed
the
feat/support-for-entryfiles
branch
from
6b0d75d to
ac9103c
Compare
rainerhahnekamp
left a comment
rainerhahnekamp
left a comment
There was a problem hiding this comment.
Thanks so much for your patience throughout this process — I really appreciate it. I know we had multiple rounds of review, and it took me way too long to get back to you each time. Thanks again for bearing with me.
I’ve now gone through the code — only a few very minor things, so I believe this will be the final review on my end. Really solid work overall!
Just two final points:
1. Integration tests: I haven’t looked at them yet, but I don’t think that should block the PR.
2. Git rename issue: About the file rename (get-entry-from-cli-or-config → get-entries-from-cli-or-config) — I tried multiple approaches (and spent quite a bit of time on it), but none worked reliably.
To avoid further issues, I’d suggest splitting this into two merges:
• First, a separate PR that only renames the file.
• Then, rebase this branch on top of that so we can merge it cleanly.
Let me know what you think — and again, thanks a lot for your great work and your patience!
|
|
||
| Run `npx sheriff init` to create a `sheriff.config.ts`. Its configuration runs with [automatic tagging](./dependency-rules#automatic-tagging), meaning no dependency rules are in place, and it only checks for the module boundaries. | ||
|
|
||
| ## `verify [main.ts]` |
There was a problem hiding this comment.
While we only had entryFile, it felt acceptable to briefly mention it in a sentence at the end.
Now, with the introduction of entryPoints, the situation has become a bit more complex. I think it would be better to dedicate a separate chapter at the end of the page that explains both entryFile and entryPoints. This would allow us to elaborate on their usage and provide examples.
We could then also keep the verify, list, and export sections more concise by simply referring to this new chapter.
|
|
||
| const data = getProjectData(entryFile, fs.cwd(), { includeExternalLibraries: true }); | ||
| cli.log(JSON.stringify(data, null, ' ')); | ||
| for (const entry of projectEntries) { |
There was a problem hiding this comment.
Yes, you are right. Sorry for the confusion.
We need to have the possibility to pass on the projectName as well (if there are more projects involved).
michaelbe812
force-pushed
the
feat/support-for-entryfiles
branch
from
1600692 to
4c5c873
Compare
chore(core): add tests for list in case of entryPoints refactor(core): fix case for parse-config
…etween comma separated entryPoints given from CLI
michaelbe812
force-pushed
the
feat/support-for-entryfiles
branch
from
ea55f09 to
49d2c47
Compare
|
rainerhahnekamp
merged commit 6adee40
into
softarc-consulting:main
2 participants
PR for implementing support for multi-project workspaces.
Feature
Running
sheriff listin a multi-workspace project like the test-project angular-v-multi will now output:This project contains 10 modules: . (root) └── projects ├── app-i └── src └── app └── non-compliant ├── data-access (type:data-access) ├── feat (type:feat) ├── types (type:types) ├── ui (type:ui) └── util (type:util) └── app-ii └── src └── app └── non-compliant ├── data-access (type:data-access) ├── feat (type:feat) ├── types (type:types) ├── ui (type:ui) └── util (type:util)For single project workspaces nothing changed.
sheriff verifywill output in case of violotions for multi-project workspaces:If no violations are found:
For single project workspaces nothing changed
Hint for Reviewer to Test
Kindly run the sheriff tasks in the new
test-projectangular-v-multi